-
-
Notifications
You must be signed in to change notification settings - Fork 77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove node references from Talon spoken form reader #2480
Conversation
Can you elaborate a bit on why you've gone this route? The idea was for this to be node-specific and move it into a separate package |
Definitely. I aim for all Cursorless features to be available in a non node environment. Since we already have a file system representation this was an easy win to share as much functionality as possible between different implementations while still supporting non node environments. |
Do you have an example of a non-node environment where we might need to do this via file system? |
Talon js for example. Can read the file via a talon action, but not via node. But I'm not against making it more abstract. |
But wouldn't talon js just get the spoken forms from talon? |
In the long run maybe, but that would require changes to cursorless-Talon. Today the source of truth for the spoken forms is a json file and it's much easier to just read that. Let's look at it from another angle. Why even have a file system abstraction if all file system interaction is gonna be via node? I'm starting to lean towards actually removing the file system interface and have more custom interfaces for the editor to provide the specific data in a more environment agnostic way. |
Yeah I'm ok with keeping the file thing in the short term if it's a lot easier, but does feel weird to use file-based rpc when you're in the same process
Exactly. That's what I was proposing Tuesday |
6efad17
to
8ff20e5
Compare
This latest version I like. Create engine gets the Talon spoken form as an argument and vscode has its own implementation. Then later in a separate pull request we can I remove the file system abstraction since that is used in other places as well that needs to be updated. |
update from meet-up:
|
@pokey ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good but just want to make sure you had a chance to try this locally; see comment below
packages/cursorless-engine/src/disabledComponents/DisabledTalonSpokenForms.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Checklist